Skip to content

C++: Remove TInitializeThisValueNumber from IR value numbering#3629

Merged
dbartol merged 4 commits into
github:masterfrom
MathiasVP:remove-initialize-this-from-value-numbering
Jun 5, 2020
Merged

C++: Remove TInitializeThisValueNumber from IR value numbering#3629
dbartol merged 4 commits into
github:masterfrom
MathiasVP:remove-initialize-this-from-value-numbering

Conversation

@MathiasVP

@MathiasVP MathiasVP commented Jun 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

We stopped generating InitializeThisInstruction as of #3571, so the value numbering case for TInitializeThisValueNumber is now subsumed by TInitializeParameterValueNumber.

Now the QL compiler doesn't complain when I remove InitializeThisInstruction from Instruction.qll, so that's probably something we should do at some point. I prefer to keep that out of this PR, though.

@MathiasVP MathiasVP added the C++ label Jun 5, 2020
@MathiasVP MathiasVP requested review from a team as code owners June 5, 2020 13:07
@dbartol

dbartol commented Jun 5, 2020

Copy link
Copy Markdown

It looks like C# is still generating InitializeThis, so removing it would require a bit more work. The C# tests may or may not depend on value numbering of this. If the existing C# tests pass, I'm happy to merge this despite it potentially breaking C# in the future, since they don't have any immediate plans to use the IR.

@dbartol dbartol self-assigned this Jun 5, 2020
@MathiasVP

MathiasVP commented Jun 5, 2020

Copy link
Copy Markdown
Contributor Author

It looks like C# is still generating InitializeThis, so removing it would require a bit more work. The C# tests may or may not depend on value numbering of this. If the existing C# tests pass, I'm happy to merge this despite it potentially breaking C# in the future, since they don't have any immediate plans to use the IR.

Oh, right. I completely forgot about C#. Let's see what happens with the tests 🤞.

dbartol
dbartol approved these changes Jun 5, 2020
View reviewed changes

@dbartol dbartol left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C# tests passed, so I'll approve. When we go back to actually remove TInitializeThisInstruction, we'll have to fix up C#.

@dbartol dbartol merged commit d4e1ee8 into github:master Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@dbartol dbartol dbartol approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@dbartol dbartol

Labels

C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MathiasVP @dbartol